Make Random package not depend on global state #146
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Stefan-Hanke @sladwig Can you take a look? |
src/core/reducer.js
Outdated
| @@ -84,11 +86,12 @@ export function createGameReducer({ game, numPlayers, multiplayer }) { | |||
| } | |||
|
|
|||
| // Init PRNG state. | |||
There was a problem hiding this comment.
That comment is out of date.
src/core/flow.js
Outdated
| @@ -278,7 +278,8 @@ export function FlowWithPhases({ | |||
| const startTurn = function(state, config) { | |||
| const ctx = { ...state.ctx }; | |||
| const G = config.onTurnBegin(state.G, ctx); | |||
| const _undo = [{ G, ctx }]; | |||
| const { random, ...ctxWithoutAPI } = ctx; // eslint-disable-line no-unused-vars | |||
There was a problem hiding this comment.
I'd like to hide how random is attached to ctx. How about a function inside random.js that performs that action?
How does this scale to, say, the Events API? Does this have to be attached/stripped likewise?
There was a problem hiding this comment.
Done. The Events API will probably have a similar attach / detach mechanism for ctx.
|
Looks ok to me. I had expected you'd need to change the JSON inside index.test.js also, but then, why should it change; the |
|
Looks goot to me too, will test it later in game. |
sladwig
approved these changes
Mar 14, 2018
sedobrengocce
pushed a commit
to sedobrengocce/boardgame.io
that referenced
this pull request
Mar 21, 2018
* remove global state * update docs * remove from package * update docs * update docs * review comments * update docs/react/boardgameio.min.js
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have the following problem at the moment:
The
Randompackage is bundled intoreact.js,core.jsandclient.js. When the user calls the API, they use the version incore.js. However, when the framework tries to persist the PRNG state, it uses the version inreact.js(if using a React client). This loses the "global" state, and makes the Random package always generate the same numbers, regardless of which seed is used.Fix
This PR injects the Random API into
ctxinstead, so you would use it like:This also has the nice property that the individual move sub-reducers are now pure.
Checklist
master).